Skip to content

Conversation

@dvdchr
Copy link
Contributor

@dvdchr dvdchr commented Dec 8, 2022

Description

Since the beginning, we have stored credentials for self-hosted sites in the keychain but don't delete them when the site is removed. This PR fixes that by wiping the stored credentials on the Blog's prepareForDeletion.

I've checked the file history to see if there's a specific edge case we tried to work around by not removing the credentials, but I couldn't find any. I've also smoke-tested the app, and it works fine.

To test

There's no way to test this manually without modifying the source code. Before running the app on the Simulator, add this debug code on the WordPressAppDelegate#willEnterForeground.

// WordPressAppDelegate.swift

func applicationWillEnterForeground(_ application: UIApplication) {
    DDLogInfo("\(self) \(#function)")

    // TODO: Remove this once you're done!
    let username = "<your self-hosted username>"
    let xmlrpc = "https://<your self-hosted site>/xmlrpc.php"
    if let _ = try? SFHFKeychainUtils.getPasswordForUsername(username, andServiceName: xmlrpc) {
        print("<<DEBUG>> ⚠️ Password detected in keychain for username: \(username) in blog: \(xmlrpc)")
    } else {
        print("<<DEBUG>> ✅ Password is not found for username: \(username) in blog: \(xmlrpc)")
    }

    ...

Warning Don't forget to replace the placeholders with your actual self-hosted credentials! 🙂

  • Run the WP app with the debug code in place.
  • Log in to a dotcom account.
  • Go to the Blog List screen > Add a self-hosted site.
  • Enter the credentials matching the one added in the debug code.
  • Verify that the self-hosted site is now displayed in My Site.
  • Scroll down and tap Remove Site to remove the self-hosted site from the app.
  • Minimize the app.
  • Verify that this shows up in the console: <<DEBUG>> ✅ Password is not found for username: ...

Regression Notes

  1. Potential unintended areas of impact
    Should be none.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manually tested the changes.

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@dvdchr dvdchr added [Type] Bug Core Data Issues related to Core Data Keychain labels Dec 8, 2022
@dvdchr dvdchr added this to the 21.4 milestone Dec 8, 2022
@dvdchr dvdchr requested review from alpavanoglu and wargcm December 8, 2022 11:18
@dvdchr dvdchr self-assigned this Dec 8, 2022
@wpmobilebot
Copy link
Contributor

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19756-be49534 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19756-be49534 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@dvdchr dvdchr added Self-hosted and removed Core Data Issues related to Core Data labels Dec 8, 2022
Copy link
Contributor

@wargcm wargcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit:

@dvdchr
Copy link
Contributor Author

dvdchr commented Dec 9, 2022

Thanks @wargcm !

@dvdchr dvdchr merged commit a0f249b into trunk Dec 9, 2022
@dvdchr dvdchr deleted the fix/delete-credentials-when-blog-removed branch December 9, 2022 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants